-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update example notebooks to use in-flight JWST data #1680
Conversation
Codecov ReportBase: 87.26% // Head: 87.26% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1680 +/- ##
=======================================
Coverage 87.26% 87.26%
=======================================
Files 95 95
Lines 10091 10091
=======================================
Hits 8806 8806
Misses 1285 1285 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
notebooks/CubevizExample.ipynb
Outdated
@@ -61,7 +64,9 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"fn = download_file('https://stsci.box.com/shared/static/28a88k1qfipo4yxc4p4d40v4axtlal8y.fits', cache=True)\n", | |||
"uri = \"mast:JWST/product/jw02732-c1001_t004_miri_ch1-longmediumshort-_s3d.fits\"\n", | |||
"result = Observations.download_file(uri, base_url=\"https://mast.stsci.edu/api/v0.1/Download/file\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't think create a cache dir under the working directory? We need to add that to .gitignore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, it just downloads the files to the working directory.
@@ -333,7 +332,7 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"plt.imshow(mask.to_image(data.shape), origin='lower')" | |||
"plt.imshow(mask.to_image(data.data.shape), origin='lower')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra .data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the extra .data
I get AttributeError: 'NDData' object has no attribute 'shape'
with these images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's weird. It is supposed to be glue Data object. What has changed while I was away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. NDData having no ndim might be a design oversight. Even CCDData has ndim. I should ask astropy. See astropy/astropy#13775
notebooks/ImvizExample.ipynb
Outdated
@@ -517,7 +516,7 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"viewer.marker = {'color': 'green', 'alpha': 0.8, 'markersize': 10, 'fill': False}" | |||
"viewer.marker = {'color': 'red', 'alpha': 0.8, 'markersize': 10, 'fill': False}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the color? The point is to show a color that is different from the default (red).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried that green over Viridis colormap wasn't visible enough, but didn't realize when I changed this that it doesn't apply to the 50 random markers before, only the my_sky
markers. I'll experiment and see what a non-red color with good visibility is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided green was fine and reverted it.
notebooks/Specviz2dExample.ipynb
Outdated
"fn = download_file('https://stsci.box.com/shared/static/exnkul627fcuhy5akf2gswytud5tazmw.fits', cache=True)" | ||
"uri = \"mast:JWST/product/jw01538-o160_s00004_nirspec_f170lp-g235h-s1600a1-sub2048_s2d.fits\"\n", | ||
"result = Observations.download_file(uri, base_url=\"https://mast.stsci.edu/api/v0.1/Download/file\")\n", | ||
"fn = os.path.join(os.path.abspath('.'), uri.rsplit('/', 1)[-1])" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what this is trying to do.
Moved to Ready for Review. @pllim I kept the ACS example in a separate notebook, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a few things with these data sets:
- cubeviz: setting collapse function to median shows a flat line (for the entire cube) - are all JWST cubes median-normalized? If not, maybe we should pick one that isn't so selecting median actually shows something
- mosviz: the first few entries (non clear/prism) are not displaying a 1D spectrum, which can be confusing for a new user. The image viewer is also blank.
- mosviz niriss: beautiful image, but the x-limits on the spectra are kind of annoying... maybe we should open a ticket to try to detect and automatically handle cases with so much "padding" on the spectral axis.
I forgot I had started working on MosvizExample, I need to revert that to the current version. |
There was a bug in a reference file that caused the padding. Let me see if those files have been reprocessed in MAST. |
@camipacifici I switched to using the reprocessed data, but it appears that it still has the same x-axis padding problem. @kecnry could you confirm? Pretty sure I avoided any caching issues. |
Confirmed... and I'm also getting the following traceback when zooming on the 2d spectrum (probably out of scope for this PR, but would be nice to have fixed if we're going to make this the standard example):
|
Ah sorry for not checking this. I guess they have not been reprocessed then. |
notebooks/ImvizExample.ipynb
Outdated
@@ -92,69 +102,61 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"acs_47tuc_1 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03gjq_flc.fits', cache=True)\n", | |||
"acs_47tuc_2 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03h1q_flc.fits', cache=True)" | |||
"files = ['jw02731-o001_t017_nircam_clear-f090w_i2d.fits',\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe tell people how big these files are. I am still waiting for the first file to download and I am not sure how long it is supposed to take. 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer my own question (but should still be added to the notebook narrative, I think):
jw02731-o001_t017_nircam_clear-f090w_i2d.fits
3.3GBjw02731-o001_t017_nircam_f444w-f470n_i2d.fits
789MB
In contrast, the HST/ACS files are around 161MB each (more or less).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, even though it will be less impressive, I wonder if we should stick to smaller subarrays in the example notebook... 💭
notebooks/ImvizExample.ipynb
Outdated
" ]\n", | ||
"for fn in files:\n", | ||
" uri = f\"mast:JWST/product/{fn}\"\n", | ||
" result = Observations.download_file(uri, base_url=\"https://mast.stsci.edu/api/v0.1/Download/file\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more of a question for @havok2063 -- Is there no way to have MAST cache this somewhere outside of the source checkout tree or at least let user provide the cache directory? Currently, this downloads the big files into the notebooks directory, which poses two different risks:
- If I do a
git clean -xdf
(which I do from time to time), I will have to redownload these again when I next run the notebook. - If someone accidentally added these to version control (you can overwrite
.gitignore
ignore if you are not careful though not easy but also not impossible), we will have to do some crazy git-fu to squash it out of git history (especially if it is not caught early).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can specify a location of the download path with local_path
, and you can set a cache
boolean flag. If you are downloading files from the production domain, i.e.mast.stsci.edu
, then you do not need to specify the base_url
keyword. See https://astroquery.readthedocs.io/en/latest/api/astroquery.mast.ObservationsClass.html#astroquery.mast.ObservationsClass.download_file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In that case, I propose we add a local_path=Path.home() / 'mastDownloads'
input to the Observations.download_file()
calls. This is Windows-friendly, I checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@havok2063 It seems that local_path expects a filename, not a directory, contrary to the documentation. Should I report this as an issue in astroquery
?
[Failed]
WARNING: Found cached file /Users/rosteen/MAST_Downloads with size 64 that is different from expected size 603515520 [astroquery.query]
---------------------------------------------------------------------------
IsADirectoryError Traceback (most recent call last)
Cell In [4], line 11
9 for fn in files:
10 uri = f"mast:JWST/product/{fn}"
---> 11 result = Observations.download_file(uri, local_path=Path.home() / 'MAST_Downloads/', cache=True)
12 with warnings.catch_warnings():
13 warnings.simplefilter('ignore')
File ~/opt/anaconda3/envs/viz_dev/lib/python3.10/site-packages/astroquery/mast/observations.py:568, in ObservationsClass.download_file(self, uri, local_path, base_url, cache, cloud_only)
565 self._download_file(data_url, local_path,
566 cache=cache, head_safe=True, continuation=False)
567 else:
--> 568 self._download_file(data_url, local_path,
569 cache=cache, head_safe=True, continuation=False)
571 # check if file exists also this is where would perform md5,
572 # and also check the filesize if the database reliably reported file sizes
573 if (not os.path.isfile(local_path)) and (status != "SKIPPED"):
File ~/opt/anaconda3/envs/viz_dev/lib/python3.10/site-packages/astroquery/query.py:437, in BaseQuery._download_file(self, url, local_filepath, timeout, auth, continuation, cache, method, head_safe, **kwargs)
433 progress_stream = io.StringIO()
435 with ProgressBarOrSpinner(length, f'Downloading URL {url} to {local_filepath} ...',
436 file=progress_stream) as pb:
--> 437 with open(local_filepath, open_mode) as f:
438 for block in response.iter_content(blocksize):
439 f.write(block)
IsADirectoryError: [Errno 21] Is a directory: '/Users/rosteen/MAST_Downloads'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, I missed this. Yeah I would report this as a bug in astroquery.
notebooks/ImvizExample.ipynb
Outdated
@@ -351,7 +353,7 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"c = SkyCoord('00h24m07.33s -71d52m50.71s')\n", | |||
"c = SkyCoord('10h36m49.211s -58d35m56.871s')\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have this actually center on some feature on the image rather than just area of blank sky.
notebooks/ImvizExample.ipynb
Outdated
@@ -497,8 +499,8 @@ | |||
"source": [ | |||
"# Add 100 randomly generated X,Y (0-indexed) w.r.t. reference image\n", | |||
"# using default marker properties.\n", | |||
"t_xy = Table({'x': np.random.randint(0, 4096, 100),\n", | |||
" 'y': np.random.randint(0, 2048, 100)})\n", | |||
"t_xy = Table({'x': np.random.randint(0, 14000, 50),\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have to take the markers stuff out or delete the subsets first before doing this. It is taking too long on my machine with these new images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced this to 10 markers but still too slow on my machine.
notebooks/ImvizExample.ipynb
Outdated
"acs_47tuc_1 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03gjq_flc.fits', cache=True)\n", | ||
"acs_47tuc_2 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03h1q_flc.fits', cache=True)" | ||
"files = ['jw02731-o001_t017_nircam_clear-f090w_i2d.fits',\n", | ||
" #'jw02731-o001_t017_nircam_clear-f187n_i2d.fits',\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we even give so many in the list? Someone is going to try to uncomment everything and their machine is going to 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I only see small performance issues when loading and viewing all of these files. And hopefully those will be improved soon. I think it's useful to have them for anyone who wants to try, but maybe a warning saying they're large files and performance may be affected depending on system specs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm updating to use the Cartwheel images instead, which are ~600 MB instead of 3.2 GB.
Co-authored-by: Duy Tuong Nguyen <[email protected]>
0682eda
to
44abc78
Compare
"# Imviz Demonstration Notebook\n", | ||
"\n", | ||
"\n", | ||
"This notebook demonstrates the Imviz API in the Notebook setting. UI equivalents for these actions, as well as additional documentation about Imviz, can be found here: https://jdaviz.readthedocs.io/en/latest/imviz/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want to provide a further description of why this notebook is different than the other ImvizExample/why we provide both?
@@ -395,7 +406,7 @@ | |||
"outputs": [], | |||
"source": [ | |||
"# Center the image on given pixel position.\n", | |||
"viewer.center_on((1173, 1013)) # X, Y (0-indexed)" | |||
"viewer.center_on((1900, 2500)) # X, Y (0-indexed)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just running through the notebook, I'm getting an error on Imviz saying these coords are "Out of Bounds". Anyone else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh good call, this will fail if you've blinked and have data B
active because it's smaller. I'll add a note about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these values need to be updated as well? At least it doesn't show anything on my screen (Sorry I couldn't get GitHub Review to link to the lines directly): https://github.com/spacetelescope/jdaviz/pull/1680/files#diff-233b3446c206e78d8c837f1c0116c834df2d3193951f6e06d9e56b1a489fd575R169-R171
I can't see what you were talking about here, but I just pushed an update to a couple things to do with the second viewer - is that what you meant? |
Huh, that link didn't work. My apologies. I was trying to reference the zoom values in the |
Ahh good point! Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I think this is my last review comment; thanks for your patience! Are the markers showing up for you in ImvizExample
? The cell is executing, but I can't find the marks. I can't tell if Im just looking in the wrong spot, or if they're not showing at all
"import pathlib\n", | ||
"\n", | ||
"example_data = 'https://stsci.box.com/shared/static/9lkf5zha6zkf8ujnairy6krobbh038wt.zip'\n", | ||
"example_data = 'https://stsci.box.com/shared/static/k94suj9yv9vdbusmcdrcodhxt9urwevr.zip'\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr -- Why not MAST download here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the full observation has a ton of objects in the catalog and is way too slow to load. This is a version of the data made by @camipacifici that cuts down the number of objects to something manageable.
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
"t_xy = Table({'x': np.random.randint(0, 4000, 20),\n", | ||
" 'y': np.random.randint(0, 4000, 20)})\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less impressive but works better for me. Otherwise, it is still laggy and gives me IOPub errors even with glue-viz/glue-jupyter#325 installed.
"t_xy = Table({'x': np.random.randint(0, 4000, 20),\n", | |
" 'y': np.random.randint(0, 4000, 20)})\n", | |
"t_xy = Table({'x': np.random.randint(0, 1000, 10),\n", | |
" 'y': np.random.randint(0, 1000, 10)})\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the most important part of the notebook! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this in so we can start using them and tweak if necessary later. Thanks!
Wait, wait, do we want a note about |
I'll add a note about it in the relevant places. |
Test failure unrelated, merging. |
Does what it says on the tin. Opening as draft since I've only finished Specviz, Cubeviz and Imviz.